Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[common] Add Canonical Serialization Spec #622

Merged
merged 4 commits into from Aug 27, 2019
Merged

Conversation

davidiw
Copy link
Contributor

@davidiw davidiw commented Aug 19, 2019

Same as title. This defines the spec and gives concrete examples for
RawTransaction and its contents.

Motivation

Inform those that write clients how to implement the specification for their own domain.

Test Plan

Contains examples and those examples are embedded within new unit tests.

Related PRs

#606

@davidiw
Copy link
Contributor Author

davidiw commented Aug 19, 2019

Please see how the markdown looks here: https://github.com/davidiw/libra/tree/serialization/common/canonical_serialization

@davidiw davidiw force-pushed the serialization branch 3 times, most recently from e4b5264 to d6d482b Compare August 20, 2019 18:44
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
common/canonical_serialization/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments on #606 (review) which actually apply here.

Copy link
Contributor

@kphfb kphfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just a couple minor comments. I'd like another set of eyes on it since it's such a large diff though. Would also like Rain/Brandon to look at the macro usage. But overall, I accept

common/canonical_serialization/src/simple_deserializer.rs Outdated Show resolved Hide resolved
common/canonical_serialization/src/simple_serializer.rs Outdated Show resolved Hide resolved
@davidiw
Copy link
Contributor Author

davidiw commented Aug 26, 2019

@bmwill, @sunshowers, @metajack

I added you as reviewers for my usage of rust macros. Feedback is welcome :).

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I left a few comments, how much investment in macros you're willing to go for would help fine-tune their relevance.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to keep the macros to the impl rather than to the trait.

macro_rules! impl_canonical_serializer {
($(($function:ident, $type:ty),)+ $(tuple: ($tuple_function:ident, $($tuple_type:ident)+),)+) => (
/// Trait for serializers that implement LCS
pub trait CanonicalSerializer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as for the macro on pub trait CanonicalDeSerializer.

@diem diem deleted a comment from huitseeker Aug 27, 2019
Same as title. This defines the spec and gives concrete examples (also
implemented in Rust) for RawTransaction and its contents.
Given the importance of canonical serialization within the code base, we
are restructuring it to make it to simplify access and understanding.

This diff moves each major component into its own separate module (file)
and re-exports so as to not break any dependencies.

In addition, major types are now fully exposed and implemented, namely
signed integers.
Macros reduce the duplicate code.

Some challenges arise though:
- macros cannot exist within a trait or an impl therefore the macro has
to wrap around it so we avoid macros for traits
- macros do not support concatenation of inputs, so the entire function
name must be specified
- macros involving indexes of tuples are just plain complicated and
probably less readable for our purposes
- tuple of size 1 is colliding with all types -- needs more
investigation
Proptest allows for broader testing than fixed sets of test vectors.
This diff migrates to proptest and also covers all of the types within
LCS which did not have coverage before.
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos on the excellent, rigorous work on a relatively ungrateful task! Thanks @davidiw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants